Repository navigation
Conversation
…ncurrent import
When a CommonJS require() targets an ESM module that a concurrent static
import already started fetching, the registry entry exists with status
Fetching and no record yet (its transpiler-thread fetch is still in
flight). loadModuleSync's top-level loadModule path reuses that pending
async fetch promise instead of forcing a synchronous fetch, so the load
never completes synchronously and require() wrongly reports the plain
(non-top-level-await) module as an unsupported async module:
TypeError: require() async module "..." is unsupported. use "await import()" instead.
This regressed in the module-loader rewrite (#29393); 1.3.11 returned the
module. It reproduces with firebase-admin's auth path, where jwks-rsa
require()s jose while the app also imports jose directly.
functionEsmLoadSync now forces the synchronous fetch for an entry that is
still Fetching with no record, mirroring the dependency-load handling in
JSModuleLoader::hostLoadImportedModule, so the entry reaches Fetched
before loadModuleSync links and evaluates it. A module that genuinely
cannot complete synchronously (real top-level await, async onLoad plugin)
leaves the fetch promise pending and still reaches the async-module throw.
|
Updated 7:53 AM PT - Jul 1st, 2026
❌ @robobun, your commit 8ff98bf has 7 failures in
🧪 To try this PR locally: bunx bun-pr 33184That installs a local version of the PR into your bun-33184 --bun |
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThis PR adds a synchronous recovery path in ChangesSync ESM load race fix and regression test
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/jsc/bindings/ZigGlobalObject.cpp`:
- Around line 770-780: Shorten the comment in ZigGlobalObject::loadModuleSync to
durable local intent only. Remove the bug-history and control-flow narrative,
and keep just the invariant that a module must be driven to Fetched before
synchronous linking/evaluation, plus the fallback that genuinely async modules
(top-level await or async onLoad) remain pending and hit the existing throw
path. Keep it to at most 3 lines.
In `@test/regression/issue/33180.test.ts`:
- Around line 5-12: Shorten the regression rationale comments in the test so
they stay within the 3-line limit and avoid the long bug-history narrative.
Update the explanatory block near the top of the file, keeping only the
essential scenario around the plain ESM module, the CommonJS require() sibling,
and the concurrent transpile fetch; preserve the intent without the detailed
loadModuleSync history.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: bce75e83-2f00-479e-826f-60a197d3dcb4
📒 Files selected for processing (2)
src/jsc/bindings/ZigGlobalObject.cpptest/regression/issue/33180.test.ts
|
Thanks, but I don't think this PR fixes #30014 or #10083. Those are a different mechanism: an async This PR only rescues modules that are synchronously completable (a plain on-disk module whose transpiler-thread fetch is merely still in flight). It drives that fetch via Verified against the exact #30014 repro on this branch, it still throws: So I'm leaving those two out of the description. They need separate handling for the async-plugin path (Node surfaces that case as |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Update: the full run (build 67577) settled with a broad, scattered set of red lanes, and it's unrelated to this diff. Each platform fails on a different subsystem:
Different tests per platform, dominated by timeouts / OOM / GC-timing / docker, is the signature of CI infra flake rather than a code defect. This change is a narrow module-loader fix ( I've already spent my one retrigger, so I'm not pushing more. Flagging for a maintainer: the diff is ready; a re-run on healthier agents should clear these. |
There was a problem hiding this comment.
No issues found and my earlier feedback was addressed, but this directly manipulates JSC module-registry promise state on a hot require(esm) path while a concurrent async fetch is still in flight — worth a look from someone close to the #29393 module-loader rewrite.
Extended reasoning...
Overview
This PR adds a ~30-line recovery path inside functionEsmLoadSync in src/jsc/bindings/ZigGlobalObject.cpp: when require(esm) finds the registry entry already at Fetching with no record (because a concurrent static import kicked off an async transpiler-thread fetch), it swaps in a SynchronousModuleQueue, re-issues loader->fetch (which routes to fetchESMSourceCodeSync while the queue is set), transfers the resulting promise state into the entry's existing fetchPromise, drains the queue, and restores the prior queue pointer. A new regression test (test/regression/issue/33180.test.ts) reproduces the race with 4 concurrent spawn attempts.
Security risks
None identified. The change is confined to internal module-loading control flow; no user-controlled input reaches new parsing/allocation logic, and no auth/crypto/permissions surfaces are touched.
Level of scrutiny
High. The module loader is one of the most correctness-critical paths in the runtime — bugs here manifest as user startup failures or hangs. This change:
- Directly calls
fulfillPromise/rejectPromiseon a registry entry'sfetchPromisewhile the original async fetch for that same entry is still in flight on the transpiler thread. Whether the later async completion racing against an already-settled internal promise is fully benign (JSC internal promises should ignore double-settlement, but the entry's status transition and any reactions attached by the concurrent import path also matter) is the kind of question that needs eyes from whoever owns the #29393 rewrite. - Issues a duplicate fetch (sync path) alongside the in-flight async one — presumably the async result is discarded, but that's an implicit contract worth confirming.
The SynchronousModuleQueue push/drain/pop shape matches the four existing sites in ModuleLoader.cpp exactly, and queue.prev is restored before the RETURN_IF_EXCEPTION, so the stack-scoped queue lifetime looks correct on all exit paths.
Other factors
- The bug-hunting system found no issues.
- My earlier inline suggestion (use
test.concurrentfor the 4 independent spawn attempts) was applied in 4a6c22f. - The regression test follows harness conventions (tempDir, bunEnv, concurrent pipe drain, exitCode asserted last) and the PR description reports guardrail tests (
require-esm-transitive-tla,issue/24387, etc.) still pass, so the TLA/async-plugin throw path is preserved. - CodeRabbit's comment-length nits were addressed and resolved.
Given the critical path and the subtle concurrent-fetch/promise-settlement interaction, I'm deferring rather than approving.
|
Thanks for the careful read. To confirm the two contracts you flagged: Double-settlement of the fetch promise is benign. The concurrent import attaches its async fetch to the entry's The fetch-settled reaction runs once. The duplicate transpile is discarded. The synchronous fetch ( Guardrail is unchanged: a module that cannot complete synchronously (real top-level await, async |
|
Independently hit the same root cause from a different entry point and converged on the same fix (settle the Two things from that branch that might be worth folding in: A deterministic, race-free reproduction. The shape in // entry.mjs
import "./starter.cjs";
import a from "./a.cjs";
import { check } from "./b.mjs";
if (a.late !== "late" || check() !== "late") throw new Error("lost");
// starter.cjs
require("./a.cjs");
// a.cjs
exports.early = "early";
require("./b.mjs"); // <- throws require() async module "b.mjs" before the fix
exports.late = "late";
// b.mjs
import a from "./a.cjs";
export function check() { return a.late; }The ESM<->CJS cycle contract. Once the false error is gone, |
|
Verified another independently reported shape of this race against current main (f972c28): a CommonJS module evaluated eagerly during module-graph loading, requiring several ESM siblings of its importer while they are still on the transpiler pool. // e1.mjs..e4.mjs: plain ESM, no top-level await
// k.cjs: const a = require("./e1.mjs"); ...through... require("./e4.mjs");
// m0.mjs: import "./e1.mjs"; ...; import k from "./k.cjs";With 1000-export siblings, an unpatched debug build at f972c28 fails 10/10 with |
|
Cross-link: #37187 fixes the same underlying gap (a top-level synchronous load reusing a mid-flight entry's pending fetch promise) reached from a different, deterministic reproduction: a CJS module inside a require()d ESM graph requiring an in-flight ESM sibling, which ended in a CommonJS module being silently skipped (no error, exit 0) rather than the TypeError. I verified this PR's change alone also fixes that silent-skip repro, so the two pre-settle hunks in functionEsmLoadSync overlap and will conflict textually; whichever lands second rebases that hunk. #37187 additionally fixes the require-cache half (a replayed makeModule fabricating an empty module after eviction, which swallows real module errors), which is independent of this PR. |
|
Consolidated into #37185, which fixes the same race one layer earlier: |
Fixes #33180
Problem
Under Bun 1.3.14+,
require()of a plain ESM module (no top-level await) throws a false positive when that module is concurrently loaded by a staticimport:The reporter hits this on startup with
firebase-admin(whosejwks-rsadoesrequire('jose')) while the app alsoimportsjosedirectly.jose's webapi entry has zero top-level await, so Node and Bun 1.3.11 both return it; 1.3.14+ throws. It regressed in the module-loader rewrite (#29393).Cause
When the
require()lands, a concurrent static import has already created the registry entry and kicked off an async fetch on the transpiler thread, so the entry is at statusFetchingwith no module record yet.require(esm)callsJSModuleLoader::loadModuleSync, whose top-levelloadModulereuses the existing pending async fetch promise (status != New) instead of forcing a synchronous fetch. The synchronous-fetch handling only exists in the dependency path (hostLoadImportedModule), not this top-level path, so the drain completes nothing and the load stays pending, whichfunctionEsmLoadSyncreports as an unsupported async module.Instrumented state at the throw (releases, synthetic repro): the entry is
Fetching, recordnull, and fetch/module/load promises are all still pending both before and afterloadModuleSync, i.e. the call advances nothing.Fix
functionEsmLoadSyncnow detects an entry that is stillFetchingwith no record and drives the fetch synchronously (set up the synchronous module queue, re-issueloader->fetchwhich routes tofetchESMSourceCodeSync, fulfill the entry's fetch promise, drain the queue), mirroring the dependency-load handling inhostLoadImportedModule. The entry then reachesFetchedand the existingloadModuleSyncpath links and evaluates it.A module that genuinely cannot complete synchronously (real top-level await, async
onLoadplugin) leaves the fetch promise pending and still reaches the async-module throw, so the existing guardrail behavior is preserved.Verification
test/regression/issue/33180.test.ts: a plain ESM module statically imported alongside a CJS sibling thatrequire()s it. Fails on the unfixed debug build with the exact issue error on every attempt; passes with the fix.require-esm-transitive-tla,require-esm-microtask-order,issue/24387(TLA still throwsERR_REQUIRE_ASYNC_MODULE), plusrequire,concurrent-dynamic-import,esModule.